-
Notifications
You must be signed in to change notification settings - Fork 97
Fix/68518 - Strike through is not applied for quote markdown text #721
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
jmusial
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @dmkt9 thanks for the PR and tests!
I think this can be solved with css though:
add
[class^='react-native-live-markdown-input'] *[data-type='strikethrough'] > [data-type='blockquote'] {
text-decoration: inherit;
}to src/web/MarkdownTextInput.css and no js changes needed.
Issue comes from blockquote being inline-block and not inheriting text-decoration by default.
|
@jmusial Thanks for your review. In my proposal I also suggested using CSS; however, that approach requires we to apply
|
|
Looking @ the issue and your proposal, indeed you suggested css solution as an alternative, but I do not see negative effects of CSS solution. AFAIK JS solution has a downside of creating an extra array and searching through it on every keystroke which for large inputs may have some negative performance impact (for larger inputs) :(. @ikevin127 are you ok with an alternative fix ? modified css to work with more than direct decendant [class^='react-native-live-markdown-input'] [data-type='strikethrough'] * {
text-decoration: inherit;
}
|
With your approach, I think it will work well. Just waiting to hear what @ikevin127 thinks. |
|
CSS is fine with me given the performance concerns of the JS solution. I think tests might need to be adjusted if we go the CSS way, but other than than if it works, that's fine by me 👍 |
|
@jmusial @ikevin127 Thank you. I will update this PR soon. |
|
Thanks for your patient. I have just updated this PR. |
jmusial
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
@dmkt9 merged, thank you for your contribution! |


Explanation of Change
Fixed Issues
$ Expensify/App#68518
PROPOSAL: Expensify/App#68518 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
The issue doesn't occur.
Android: mWeb Chrome
iOS: Native
The issue doesn't occur.
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop